Skip to content

feat: test core_resources_rotation #1288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

feat: test core_resources_rotation #1288

wants to merge 14 commits into from

Conversation

0xmovses
Copy link
Collaborator

@0xmovses 0xmovses commented Aug 1, 2025 •
edited
Loading

Pull Request Overview

This PR adds testing infrastructure for core resources rotation functionality in the Movement framework. It includes a new test release module that reuses the pre-l1-merge commit with rotated core-resource-account settings and adds an end-to-end test for core resources rotation.

  • Created a new framework release module for testing core resources rotation
  • Added process composition configuration for the test environment
  • Integrated a new e2e test binary for core resources rotation

Changelog

File Description
protocol-units/execution/maptos/framework/releases/core-resources-rotation/src/lib.rs Main library file defining the pre-l1-merge release configuration for rotation testing
protocol-units/execution/maptos/framework/releases/core-resources-rotation/src/cached.rs Cached release implementation with gas upgrades, script modules, and feature flags
protocol-units/execution/maptos/framework/releases/core-resources-rotation/Cargo.toml Package configuration and dependencies for the core resources rotation test module
process-compose/movement-full-node/process-compose.test-core-resources-rotation.yml Process composition configuration for running the core resources rotation test

Testing

  1. Test core_resources account rotation.
just movement-full-node native build.setup.eth-local.test-core-resources-rotation --keep-tui
  1. Test governance signing after core_resource rotation:
    IMPORTANT
    Remember to rm -rf .movement/ before each run. Otherwise you'll get errors.
just movement-full-node native build.setup.eth-local.test-upgrade-framework-rotated-key --keep-tui

Notes

  • More thorough checks can be done on core_resources capabilities if we want after this PR
  • I'm aware that there is some code repetition in this PR. I did attempt to make the core_resource rotation a utility that could be shared across the repo but ran into lots of circular dependency issues, figured this isn't worth the time to solve as this repo will be essentially archived soon anyway. Repetition is fine.

@Copilot Copilot AI review requested due to automatic review settings August 1, 2025 14:36
Copilot

This comment was marked as outdated.

Sign in to view

@0xmovses 0xmovses marked this pull request as draft August 1, 2025 14:36
@0xmovses 0xmovses mentioned this pull request Aug 1, 2025
Closed
@0xmovses 0xmovses requested a review from Copilot August 1, 2025 15:14
Copilot

This comment was marked as outdated.

Sign in to view

@0xmovses 0xmovses requested a review from Copilot August 4, 2025 13:49
Copilot
Copilot AI reviewed Aug 4, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds comprehensive testing infrastructure for core resources rotation functionality in the Movement framework. It introduces a new test module that reuses the pre-l1-merge commit configuration with rotated core-resource-account settings and provides end-to-end testing capabilities.

  • Created a dedicated framework release module for testing core resources rotation
  • Added process composition configuration files for different test scenarios
  • Integrated new e2e test binaries for core resources rotation testing

Reviewed Changes

Copilot reviewed 8 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
protocol-units/execution/maptos/framework/releases/pre-l1-merge/Cargo.toml Updated package description for clarity
protocol-units/execution/maptos/framework/releases/core-resources-rotation/src/lib.rs Main library defining pre-l1-merge release configuration for rotation testing
protocol-units/execution/maptos/framework/releases/core-resources-rotation/src/cached.rs Cached release implementation with gas upgrades, script modules, and feature flags
protocol-units/execution/maptos/framework/releases/core-resources-rotation/build.rs Build script for framework release generation
protocol-units/execution/maptos/framework/releases/core-resources-rotation/Cargo.toml Package configuration and dependencies for the test module
process-compose/movement-full-node/process-compose.test-upgrade-framework-rotated-key.yml Process composition for framework upgrade with rotated keys
process-compose/movement-full-node/process-compose.test-core-resources-rotation.yml Process composition for core resources rotation testing
networks/movement/movement-client/Cargo.toml Added new e2e test binary for core resources rotation
Comments suppressed due to low confidence (1)

protocol-units/execution/maptos/framework/releases/core-resources-rotation/build.rs:5

  • The struct name BiarritzRc1 is inconsistent with the PreL1Merge struct used in lib.rs. Consider using consistent naming across the module.
	BiarritzRc1,                                         // Struct name

@0xmovses
Copy link
Collaborator Author

0xmovses commented Aug 12, 2025 •
edited
Loading

Some notes on this PR. I've found that after rotating the keys (successfully) for the core_resources account it is unable to sign off on the proposal, the VM reverts with EXCEEDED_MAX_TRANSACTION_SIZE Error. However, When sign the same governance proposal with an unrotated core_resources account, it passes. So I'm led to believed the VM error is reverting with that error but the error is some other reason. I've spent a lot of time trying to figure out why, but with no luck.

My Conclusion Is

It is not safe to rotate the core_resource signer and then attempt to sign off on governance proposasl with the rotated core_resource account.

I propose that

We use the current account to proceed with all the framework signing and do not rotate this key.

If anyone is curious the code for the test and the cmds to run those tests are all in the PR description above.

@0xmovses 0xmovses marked this pull request as ready for review August 12, 2025 15:19
@0xmovses 0xmovses mentioned this pull request Aug 12, 2025
Open
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@andygolay andygolay Awaiting requested review from andygolay

@musitdev musitdev Awaiting requested review from musitdev musitdev is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@0xmovses